fix(compose): preserve project env vars when compose specifies empty passthrough placeholders - #616
Merged
Conversation
…passthrough placeholders
Do not let empty inline compose environment values (originating from
unpopulated compose passthrough expressions like ${VAR:-} or ${VAR})
clobber configured non-empty project-level environment variables during
deploy env merge.
Fixes oblien#614
…n step Review follow-ups on the oblien#614 fix. - Move the layering to `compose/service-env-layers.ts` and give the rule a name (`inlineEmptyDefers`). Two surfaces spelled it out by hand; they now import it. - Log every substitution. The service Env tab and the deploy wizard both go on showing the empty value the merge ignored, so the deploy log was the only surface that could explain the container — and it said nothing. Names only, never values, matching `resolveEnvPublicUrls` just below it. - `describeRestorePlan` built its per-service overrides as a raw spread of the service row, under a comment asserting it matched the deploy merge. It no longer did, so the rollback confirm dialog reported a `frozen-wins` revert plus a `scopeAmbiguous` warning for every compose passthrough key the rollback leaves alone. It applies the same deferral now. - Correct the docstring, which still stated the old flat precedence, and write down what the rule costs: an authored empty no longer clears a configured value, and an empty service-scoped row is the escape hatch. - Tests for the frozen layer (the combination every non-rollback deploy has), the rollback replay, the accepted trade-off, the escape hatch, and the gap a value-shaped rule cannot close — `postgres://${U}:${P}@db/${D}` resolves to non-empty garbage and still wins. - Note in `getAppConnectionView` that it is NOT the deploy merge: it omits the project layer entirely. Adding it changes what every existing app connection resolves to, so that is left for its own change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
When a Docker Compose file defines environment variables using passthrough or default syntax (such as
MY_VAR: ${MY_VAR:-}orMY_VAR: ${MY_VAR}), the compose parser imports these unset variables into the service's inline environment as""(empty string).During deployment,
mergeServiceDeployEnvlayersproject -> inline -> service. Because the inline environment hadMY_VAR: "", it clobbered any configured non-empty project-level environment variable with an empty string.Closes #614
Solution
mergeServiceDeployEnv, when merging the inline compose environment over the project environment, skip empty string values ("") if the project environment already defines a non-empty value for that key.PORT: "3000") and explicit service-scoped environment overrides still take precedence as intended.apps/api/test/modules/deployments/compose-env-passthrough.test.tsto verify and defend this behavior.